Ensure NoJump handles jumps that occur on the second frame in NPT trajectories #4258

p-j-smith · 2023-08-26T09:03:31Z

Changes made in this Pull Request:

ensure jumps across periodic boundaries that occur at the second frame are accounted for by mda.transformations.NoJump
added two tests:
- test_nojump_2nd_frame.- tests that jumps that occur on the second frame are handled correctly
- test_nojump_3rd_frame.- tests that jumps that occur on the third frame are handled correctly
the test_nojump_3rd_frame test isn't necessary and can probably be removed. However, I've included it here to illustrate the the fix in this pr is necessary. Before this pr, test_nojump_3rd_frame would pass but test_nojump_2nd_frame would fail, and the only difference between the trajectories is the frame at which the jump across a boundary occurs
the fix requires that we know the atom positions and box size at the first frame of the trajectory, and so I've added an ag argument that takes an AtomGroup to be unwrapped. This however would be a breaking change to the api. Perhaps a better way would be to add an if-statement to mda.transformations.NoJump._transform that checks if we're on the first frame of the trajectory, and if so then sets the appropriate values for NoJump.prev, NoJump.old_frame, and NoJump.older_frame?
updated all other tests for nojump to use the required ag argument

~~Note, I've temporarily put the test datasets in the testsuite itself so the tests can run before the datasets are added to MDAnalysisData~~

Edit: I was running into some issues with CI failing because the test datafiles I added weren't present in MDAnalysisData. So I've added a tranformation mda.transformations.boxdimensions.set_variable_dimensions to set different box dimensions at each frame (so we can create an NPT trajectory to go with mda.Universe.empty).

PR Checklist

Tests?
Docs?
CHANGELOG updated?
Issue raised/referenced?

Developers certificate of origin

I certify that this contribution is covered by the LGPLv2.1+ license as defined in our LICENSE and adheres to the Developer Certificate of Origin.

github-actions · 2023-08-26T09:05:49Z

Linter Bot Results:

Hi @p-j-smith! Thanks for making this PR. We linted your code and found the following:

Some issues were found with the formatting of your code.

Code Location	Outcome
main package	⚠️ Possible failure
testsuite	⚠️ Possible failure

Please have a look at the darker-main-code and darker-test-code steps here for more details: https://github.com/MDAnalysis/mdanalysis/actions/runs/6064286779/job/16452562369

Please note: The black linter is purely informational, you can safely ignore these outcomes if there are no flake8 failures!

codecov · 2023-08-26T11:13:06Z

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (2acd594) 93.40% compared to head (de33028) 93.40%.

Additional details and impacted files

@@            Coverage Diff             @@
##           develop    #4258     +/-   ##
==========================================
  Coverage    93.40%   93.40%             
==========================================
  Files          170      184     +14     
  Lines        22246    23358   +1112     
  Branches      4071     4071             
==========================================
+ Hits         20779    21818   +1039     
- Misses         951     1024     +73     
  Partials       516      516

Files Changed	Coverage Δ
...ackage/MDAnalysis/transformations/boxdimensions.py	`100.00% <100.00%> (ø)`
package/MDAnalysis/transformations/nojump.py	`100.00% <100.00%> (ø)`

... and 14 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

p-j-smith · 2023-08-27T19:15:51Z

the fix requires that we know the atom positions and box size at the first frame of the trajectory, and so I've added an ag argument that takes an AtomGroup to be unwrapped. This however would be a breaking change to the api. Perhaps a better way would be to add an if-statement to mda.transformations.NoJump._transform that checks if we're on the first frame of the trajectory, and if so then sets the appropriate values for NoJump.prev, NoJump.old_frame, and NoJump.older_frame?

I've removed the ag argument I added and gone with using an if-statement to check it we're on the first frame. This both avoids a breaking change and ensures the trajectory can be iterated over multiple times and unwrapping still be performed correctly. I've also added a test to check that coordinates are always unwrapped correctly when iterating multiple times.

hmacdope

Tiny little nitpicks, but other than that looking great. Don't forget CHANGELOG ;)

package/MDAnalysis/transformations/boxdimensions.py

testsuite/MDAnalysisTests/transformations/test_nojump.py

p-j-smith · 2023-08-28T07:10:52Z

thanks for the speedy review @hmacdope! And sorry for mixing a fix and a feature in the same pr - I probably should have added set_variable_dimensions separately :)

IAlibay

One initial blocking request - I'll follow up with smaller review things.

IAlibay · 2023-08-28T08:15:02Z

package/MDAnalysis/transformations/boxdimensions.py

@@ -92,3 +93,74 @@ def __init__(self,
    def _transform(self, ts):
        ts.dimensions = self.dimensions
        return ts
+
+
+class set_variable_dimensions(TransformationBase):


Could we just move this new functionality as a new option of the existing set_dimension? The order of how things are done is very similar and it would be a much better user experience to not have to deal with (choose between) a lot of classes that do very similar things.

Yep I think that would make sense

richardjgowers · 2023-08-28T08:29:21Z

package/MDAnalysis/transformations/boxdimensions.py

+        dim = [
+            [2, 2, 2, 90, 90, 90],
+            [4, 4, 4, 90, 90, 90],
+        ]


I'd use a numpy array in the example if possible

IAlibay

the "small changes" review

package/CHANGELOG

IAlibay · 2023-08-28T08:22:23Z

package/MDAnalysis/transformations/boxdimensions.py

+        try:
+            ts.dimensions = self.dimensions[ts.frame]
+        except IndexError as e:
+            raise NoDataError(f"Dimensions array has no data for frame {ts.frame}") from e


As per #3901, NoDataError is really only meant to exist within the realm of TopologyAttr. A ValueError probably should be used here.

ah okay, that's good to know

testsuite/MDAnalysisTests/transformations/test_boxdimensions.py

IAlibay · 2023-08-28T08:23:55Z

testsuite/MDAnalysisTests/transformations/test_boxdimensions.py

+    ts = variable_boxdimensions_universe.trajectory.ts
+    with pytest.raises(ValueError, match='valid box dimension shape'):
+        set_variable_dimensions(dim_vector_shapes)(ts)
+


Suggested change

testsuite/MDAnalysisTests/transformations/test_nojump.py

package/MDAnalysis/transformations/nojump.py

… on the second frame Previosuly, jumps wouldn't be unwrapped until the third frame due to the way the class was initialised. To fix the issue, the NoJump transformation now requires an 'ag' argument, which is used to determine the box size at the first frame

…nd and third frames Also update other tests to pass the 'ag' argument to the transformation

For creating NPT trajectories

…imensions are missing for a frame

…values at all frames when iterating over multiple times

over the trajectory multiple times

…ted unwrapping Also don't pass 'ag' argument to NoJump in tests

Co-authored-by: Hugo MacDermott-Opeskin <[email protected]>

Co-authored-by: Irfan Alibay <[email protected]>

It can be set to any number, just not 'A'.

…ndled correctly by NoJump

p-j-smith · 2023-08-28T09:11:18Z

Thanks for the reviews @richardjgowers and @IAlibay ! I think I've addressed all your comments, and I've just rebased on to the latest develop

IAlibay

Couple more small things, otherwise lgtm

package/MDAnalysis/transformations/nojump.py

IAlibay · 2023-08-28T09:20:45Z

package/MDAnalysis/transformations/boxdimensions.py

                       '[a, b, c, alpha, beta, gamma]')
            raise ValueError(errmsg)

    def _transform(self, ts):
-        ts.dimensions = self.dimensions
+        try:
+            ts.dimensions = self.dimensions[0] if self.dimensions.shape[0] == 1 else self.dimensions[ts.frame]


This is both a long line and also a bit hard to read (at least to me), splitting this over a few lines probably would be worth it?

IAlibay · 2023-08-28T09:22:08Z

package/MDAnalysis/transformations/boxdimensions.py

+        try:
+            ts.dimensions = self.dimensions[0] if self.dimensions.shape[0] == 1 else self.dimensions[ts.frame]
+        except IndexError as e:
+            raise ValueError(f"Dimensions array has no data for frame {ts.frame}") from e


Suggested change

raise ValueError(f"Dimensions array has no data for frame {ts.frame}") from e

errmsg = f"Dimensions array has no data for frame {ts.frame}"

raise ValueError(errmsg) from e

I know it's annoying but pep8 - note we don't care about the black linter stuff, but the flake8 errors from here probably should be addressed: https://github.com/MDAnalysis/mdanalysis/actions/runs/5997923144/job/16265248319

ah sorry I forgot to run flake8 locally, fixed now! Perhaps adding a pre-commit config for people to optionally use would be useful?

pre-commit config for people to optionally use would be useful?

It's.. complicated, I'll leave it as that for now.

ha! okay :)

package/MDAnalysis/transformations/boxdimensions.py

p-j-smith · 2023-08-28T19:05:00Z

I just realised that with the changes in this pr there are now some parts of NoJump that are not tested (because of the early return statement). I'll add tests for them tomorrow

This will never be true

…ory with stride > 1

orbeckst · 2023-08-31T19:33:16Z

@hmacdope could you be the one looking after this PR, chasing reviews if necessary, and merging, i.e., assign it to you?

Co-authored-by: Irfan Alibay <[email protected]>

hmacdope · 2023-08-31T23:16:17Z

@hmacdope could you be the one looking after this PR, chasing reviews if necessary, and merging, i.e., assign it to you?

Yep, ill re-review ASAP.

IAlibay

lgtm thanks!

hmacdope

Looks good to me @p-j-smith. Thank you so much for the detailed bug report, timely fix and detailed testing! Its really appreciated.

…t-frame

p-j-smith · 2023-09-03T13:39:17Z

Thanks for the speedy reviews everyone!

orbeckst · 2023-09-03T14:54:36Z

Thank you for the contribution, Paul. Really good work!Am 9/3/23 um 09:39 schrieb Paul Smith ***@***.***>: Thanks for the speedy reviews everyone! —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: ***@***.***>

hmacdope requested changes Aug 27, 2023

View reviewed changes

package/MDAnalysis/transformations/boxdimensions.py Outdated Show resolved Hide resolved

package/MDAnalysis/transformations/boxdimensions.py Show resolved Hide resolved

testsuite/MDAnalysisTests/transformations/test_nojump.py Outdated Show resolved Hide resolved

IAlibay requested changes Aug 28, 2023

View reviewed changes

richardjgowers reviewed Aug 28, 2023

View reviewed changes

IAlibay requested changes Aug 28, 2023

View reviewed changes

IAlibay reviewed Aug 28, 2023

View reviewed changes

package/MDAnalysis/transformations/nojump.py Show resolved Hide resolved

p-j-smith and others added 21 commits August 28, 2023 10:09

Add testfiles for nojump transformation

0a835ad

Add tests for nojump transformation where the jump occurs at the seco…

4efdf20

…nd and third frames Also update other tests to pass the 'ag' argument to the transformation

Remove unncessary second assignment of L in NoJump initialisation

4a2b8a5

Temporarily put test datafiles in the test directory

7245bd1

Add transformation to set variable box dimensions

1dae25a

For creating NPT trajectories

Use set_variable_dimensions transformation in test for NoJump

0ed530e

Remove the test datafiles for the npt NoJump

66ccb4a

Import set_variable_dimensions in the transformation sub-package

c3e8366

Fix typos in docsttring of set_variable_dimensions transformation

58a7567

remove unncessary comments in docstring of npt tests for nojump

6f02285

Add tests for set_variable_dimensions transformation

7d90706

Add test to check that set_variable_dimensions raises an error when d…

1b94f84

…imensions are missing for a frame

Tidy formatting of set_variable_dimensions test functions

970663a

Add docs for set_variable_dimensions

7ffaa2e

Add versionchanged note to NoJump about adding 'ag' argument

21fdf0a

Add test to check if the nojump transform always returns the correct …

aa86f88

…values at all frames when iterating over multiple times

ensure coordinates can be unwrapped correctly when iterating

55d3c34

over the trajectory multiple times

Use tmp_path_factory to write tmp trajectory for testing NoJump repea…

18d2bfc

…ted unwrapping Also don't pass 'ag' argument to NoJump in tests

Fix typo in boxdimensions module docstring

a786599

Co-authored-by: Hugo MacDermott-Opeskin <[email protected]>

Fix typo in docstring of NoJump test

8e06cf3

Co-authored-by: Hugo MacDermott-Opeskin <[email protected]>

p-j-smith and others added 6 commits August 28, 2023 10:09

Remove unnecessary whitespace from NoJump tests

90c037a

Co-authored-by: Irfan Alibay <[email protected]>

Use numpy array in docstring example of set_dimensions transformation

21ba7c3

Update changelog

8b3dfaf

Use ValueError rather than NoDataError in set_dimensions

44e473a

Set NoJump.older_frame to -1 when ts.frame == 0.

0d51b3d

It can be set to any number, just not 'A'.

Add comment to explain how we ensure jumps on the second frame are ha…

2a2d100

…ndled correctly by NoJump

p-j-smith force-pushed the fix/no-jump-first-frame branch from 5cbba9c to 2a2d100 Compare August 28, 2023 09:10

IAlibay requested changes Aug 28, 2023

View reviewed changes

Make flake8 happy

d735d71

p-j-smith and others added 3 commits August 29, 2023 06:55

Remove unnecessary if-block 'if self.prev is None:' from NoJump

7f87344

This will never be true

Add test to check warning is emitted by NoJump when iterating traject…

35dc77c

…ory with stride > 1

Merge branch 'develop' into fix/no-jump-first-frame

cd339b6

Fix formatting of set_dimensions docstring

b1c32dc

Co-authored-by: Irfan Alibay <[email protected]>

hmacdope self-assigned this Aug 31, 2023

Merge branch 'develop' into fix/no-jump-first-frame

9a24312

IAlibay approved these changes Sep 1, 2023

View reviewed changes

Merge branch 'develop' into fix/no-jump-first-frame

8926e4a

IAlibay requested a review from hmacdope September 2, 2023 11:40

hmacdope approved these changes Sep 3, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/develop' into fix/no-jump-firs…

de33028

…t-frame

hmacdope merged commit f50a097 into MDAnalysis:develop Sep 3, 2023

IAlibay added enhancement maintainability labels Sep 21, 2023

orbeckst mentioned this pull request Jan 4, 2024

Allow setting user box dimensions to a multidimensional array. #3217

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure NoJump handles jumps that occur on the second frame in NPT trajectories #4258

Ensure NoJump handles jumps that occur on the second frame in NPT trajectories #4258

p-j-smith commented Aug 26, 2023 •

edited

Loading

github-actions bot commented Aug 26, 2023 •

edited

Loading

codecov bot commented Aug 26, 2023 •

edited

Loading

p-j-smith commented Aug 27, 2023

hmacdope left a comment

p-j-smith commented Aug 28, 2023

IAlibay left a comment

IAlibay Aug 28, 2023

p-j-smith Aug 28, 2023

richardjgowers Aug 28, 2023

IAlibay left a comment

IAlibay Aug 28, 2023

p-j-smith Aug 28, 2023

IAlibay Aug 28, 2023

p-j-smith commented Aug 28, 2023

IAlibay left a comment

IAlibay Aug 28, 2023

IAlibay Aug 28, 2023

p-j-smith Aug 28, 2023

IAlibay Aug 28, 2023

p-j-smith Aug 28, 2023

p-j-smith commented Aug 28, 2023

orbeckst commented Aug 31, 2023

hmacdope commented Aug 31, 2023

IAlibay left a comment

hmacdope left a comment

p-j-smith commented Sep 3, 2023

orbeckst commented Sep 3, 2023 via email

	raise ValueError(f"Dimensions array has no data for frame {ts.frame}") from e
	errmsg = f"Dimensions array has no data for frame {ts.frame}"
	raise ValueError(errmsg) from e

Ensure NoJump handles jumps that occur on the second frame in NPT trajectories #4258

Ensure NoJump handles jumps that occur on the second frame in NPT trajectories #4258

Conversation

p-j-smith commented Aug 26, 2023 • edited Loading

PR Checklist

Developers certificate of origin

github-actions bot commented Aug 26, 2023 • edited Loading

Linter Bot Results:

codecov bot commented Aug 26, 2023 • edited Loading

Codecov Report

p-j-smith commented Aug 27, 2023

hmacdope left a comment

Choose a reason for hiding this comment

p-j-smith commented Aug 28, 2023

IAlibay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-j-smith commented Aug 28, 2023

IAlibay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-j-smith commented Aug 28, 2023

orbeckst commented Aug 31, 2023

hmacdope commented Aug 31, 2023

IAlibay left a comment

Choose a reason for hiding this comment

hmacdope left a comment

Choose a reason for hiding this comment

p-j-smith commented Sep 3, 2023

orbeckst commented Sep 3, 2023 via email

p-j-smith commented Aug 26, 2023 •

edited

Loading

github-actions bot commented Aug 26, 2023 •

edited

Loading

codecov bot commented Aug 26, 2023 •

edited

Loading